[ALI] adds AUTH_GH_ORG and AUTH_GH_REPO to set/restrict authentication and management for a specific org/repo #7152
Conversation
…nstead of guessing
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
meta-cla
bot
added
the
CLA Signed
| default = "" | ||
| } | ||
|
|
||
| variable "auth_gh_org" { |
There was a problem hiding this comment.
naming nit: can we have a name without "auth" in it? Maybe "ci_github_org" or just "github_org"
The auth part is rather incidental here (a gh app can be authorized to affect multiple github orgs). The key thing we want to know is which github org is this particular autoscaler fleet supposed to be managing.
There was a problem hiding this comment.
same as here https://github.com/pytorch/test-infra/pull/7152/files/d5b2cab0b9a3d87bf5c93e050a331f323e1104f6#r2356755756
there are quirks on authenticating that go beyond just managing, and I wanted to make it explicit.
There was a problem hiding this comment.
Gotcha, makes sense. Maybe just update the description in that case to explain the reasoning here. Something like: "Github organization this runner fleet is authorized to have access to"
There was a problem hiding this comment.
humm, I'll keep like this, it is the main use as the one provided when authenticating, this changes the key and installation ID for the Github APP and those are relevant informations.
It is not only the one that the fleet is authorized to, but it is the one it is authorized by... and make it very explicit seems reasonable to me.
terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/utils.ts
Show resolved
Hide resolved
terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/sqs.ts
Show resolved
Hide resolved
terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/config.ts
Show resolved
Hide resolved
terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-down.ts
Show resolved
Hide resolved
terraform-aws-github-runner/modules/runners/lambdas/runners/Makefile
Outdated
Show resolved
Hide resolved
| const validRunnerTypes = await getRunnerTypes( | ||
| // For scaleUpChron, we don't have a situation where the auth repo is different from the config repo | ||
| // so we can just pass the same repo for both parameters | ||
| repo, | ||
| repo, | ||
| scaleConfigRepo, |
There was a problem hiding this comment.
the comment up above this line should be updated
There was a problem hiding this comment.
Why not delete the comment? The one one lines 23-24 no longer apply
There was a problem hiding this comment.
ouch, sorry for missing this one
There was a problem hiding this comment.
wait it is deleted on my last commit in this PR: c8b97f6#diff-cb1d944243a46169e240d54bf00468a37203705ca8d1a24ebced70992aaa5cd2L23
…nschmidt/scaleConfigOrg_scaleUpChron
| default = "" | ||
| } | ||
|
|
||
| variable "auth_gh_org" { |
There was a problem hiding this comment.
Gotcha, makes sense. Maybe just update the description in that case to explain the reasoning here. Something like: "Github organization this runner fleet is authorized to have access to"
| const validRunnerTypes = await getRunnerTypes( | ||
| // For scaleUpChron, we don't have a situation where the auth repo is different from the config repo | ||
| // so we can just pass the same repo for both parameters | ||
| repo, | ||
| repo, | ||
| scaleConfigRepo, |
There was a problem hiding this comment.
Why not delete the comment? The one one lines 23-24 no longer apply
Currently we've been guessing org/repo to authenticate in most cases, this configuration when set both enforce validation and defaults to the given org/repo for authentication.
This enables scaleUpChron to run properly when the org/repo for the scale config is not the same as the one used to authenticate and being managed.
additional small fixes to remove warnings on linting that have been ignored.